-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add JSON as an option for log output #1654
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this! I've given it a quick skim, and have one comment
59fb3ea
to
9efb37c
Compare
thanks for the PR @carthewd! I'm wondering if it would make more sense to add a flag like |
+1 looks like logrus only supports text and json outside of external formatters, but I agree that a |
44a78c7
to
a46676d
Compare
@carthewd thanks for the update! there are some more changes that I'd like to see made, but you're under no obligation to keep refining - I'm happy to make the changes myself (you'll still be tagged on the commit and changelog). Or, if you do have the appetite, I can provide another round of comments. Totally up to you - just let me know! |
Thanks @skriss - I really appreciate the feedback! I’m quite happy to keep refining this and get it right if it’s on the right track so feel free to add more comments |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, added comments around the core change I'd like to make - some of the other code will have to update as well to compile once these changes are made, but hopefully those become clear.
The other thing is I don't think we need to add the loops in all of our tests to exercise both log formats. Let's add a single test in pkg/util/logging
that sets up a logger using both formats, and validates that the output is what we expect, and then we can remove the loops in the other tests.
Once you've made the updates, make sure to run |
Signed-off-by: Donovan Carthew <donovan.carthew@gmail.com>
Signed-off-by: Donovan Carthew <donovan.carthew@gmail.com>
41ea9c0
to
7e04d26
Compare
Signed-off-by: Donovan Carthew <donovan.carthew@gmail.com>
e1bac02
to
ec932c6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This lgtm, is it missing any more review to be addressed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great @carthewd, just 2 minor things then LGTM. Thanks for the updates!
Signed-off-by: Donovan Carthew <donovan.carthew@gmail.com>
d05e907
to
59f6b29
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
This enhancement is quite useful for my team so I took a crack at adding it. Adds an option to output logs as JSON via --log-json (e.g., velero server --log-json)
Fixes #598
Signed-off-by: Donovan Carthew donovan.carthew@gmail.com